build: Add option to build monolithic, shared library #10732
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
@assignUser, this is just a reply to the mentioned issue in PR description. In Gluten, we are using static folly lib, as well as gflags, for velox to link. Can upstream velox link these static libs also? |
|
@PHILO-HE that's what we normaly do as well but when building velox as an .so it doesn't work because of the mentioned linking issue. |
@assignUser, I see. Thanks for your clarification! |
assignUser
force-pushed
the
mono-shared
branch
2 times, most recently
from
f0b2964 to
654ad37
Compare
|
I am unclear on the one failed test, would be great if someone could have a look. But otherwise this is pretty done, we are currently using a temp CI image of course. |
assignUser
force-pushed
the
mono-shared
branch
from
654ad37 to
9ac8be2
Compare
|
I tried doing the monolithic shared build and ran into the error: |
assignUser
force-pushed
the
mono-shared
branch
from
a6afca1 to
abfd6e4
Compare
assignUser
force-pushed
the
mono-shared
branch
from
7bd649f to
b893dd7
Compare
assignUser
changed the title
assignUser
force-pushed
the
mono-shared
branch
from
fa31ce7 to
fcd2659
Compare
assignUser
force-pushed
the
mono-shared
branch
2 times, most recently
from
629ef65 to
c8bfb15
Compare
| @@ -93,13 +93,26 @@ function(velox_link_libraries TARGET) | |||
| # TODO(assignUser): Handle scope keywords (they currently are empty calls ala | |||
There was a problem hiding this comment.
nit: can we create an issue and remove the TODO
| # Together with the patch applied above prevents folly from test compiling a | ||
| # snippet to find the right namespace (which would fail because gflags isn't | ||
| # built yet) | ||
| set(FOLLY_UNUSUAL_GFLAGS_NAMESPACE OFF) |
There was a problem hiding this comment.
curious why not build gflags before folly then ?
There was a problem hiding this comment.
All of this finishes in the configure step, nothing is built only cmake is run BUT the test compilation happens in folly's cmake. So we have to work around this.
| "NOT VELOX_BUILD_SHARED" | ||
| OFF) | ||
|
|
||
| if(VELOX_BUILD_SHARED AND NOT VELOX_MONO_LIBRARY) |
There was a problem hiding this comment.
nit: If we only have VELOX_BUILD_SHARED then we should set VELOX_MONO_LIBRARY to ON if its unset , rather than throw.
There was a problem hiding this comment.
hm, might be confusing but I guess we flip the switch for necessary components of other features as well.
There was a problem hiding this comment.
Yeah, we do flip the switch in other cases. Let's set VELOX_MONO_LIBRARY to ON as Krishna suggested.
CMakePresets.json
Outdated
| @@ -0,0 +1,77 @@ | |||
| { | |||
There was a problem hiding this comment.
nice ! - Doesnt this require a comment header?
There was a problem hiding this comment.
Ah, actually I added that for testing (that's why it's bare bones). I'll pull it out, completely and add it back in in a follow up.
No there are no comments in JSON so we have to exclude it from the check (or it seems to already be).
| # These files wrap the generated source and header files from | ||
| # velox_dwio_dwrf_proto with pragamas to disable certain warnings TODO: transfer | ||
| # disabling the warnings to CMake/Buck | ||
| velox_add_library(velox_dwio_dwrf_proto orc-proto-wrapper.cpp |
There was a problem hiding this comment.
Did we not need this before ?
There was a problem hiding this comment.
They were built twice and linked inconsistently so I unified this. I did not test if we actually still need the pragmas.
assignUser
force-pushed
the
mono-shared
branch
from
010b345 to
8406bf3
Compare
| @@ -29,7 +29,7 @@ jobs: | |||
| # prevent errors when forks ff their main branch | |||
| if: ${{ github.repository == 'facebookincubator/velox' }} | |||
| runs-on: 8-core-ubuntu | |||
| container: ghcr.io/facebookincubator/velox-dev:adapters | |||
| container: ghcr.io/assignuser/velox-dev:adapters | |||
There was a problem hiding this comment.
| container: ghcr.io/assignuser/velox-dev:adapters | |
| container: ghcr.io/facebookincubator/velox-dev:adapters |
Add before merge
| "NOT VELOX_BUILD_SHARED" | ||
| OFF) | ||
|
|
||
| if(VELOX_BUILD_SHARED AND NOT VELOX_MONO_LIBRARY) |
There was a problem hiding this comment.
Yeah, we do flip the switch in other cases. Let's set VELOX_MONO_LIBRARY to ON as Krishna suggested.
| @@ -109,6 +109,22 @@ velox_link_libraries( | |||
| velox_arrow_bridge | |||
| velox_common_compression) | |||
|
|
|||
| velox_add_library(velox_cursor Cursor.cpp) | |||
There was a problem hiding this comment.
What is the failure that requires this move?
There was a problem hiding this comment.
https://github.com/facebookincubator/velox/actions/runs/11947025017/job/33302337531#step:6:3179
LocalRunner uses this, when static linking it somehow transitively found it's way in but shared linking is more stringent. The fix might be to refactor the use out of LocalRunner but that's work for someone else^^
|
Importing to check internal build. |
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: I have pulled these changes from #10732 as they are not specifically part of the shared build. - Add `[<dependency name>]` as an indicator that a dependency is running CMake. This makes it much easier to parse the log and track issues: ``` -- [simdjson] Using BUNDLED simdjson -- Setting folly source to BUNDLED -- [folly] Building Folly from source -- [folly] Using Boost - Bundled ``` - use targets instead of variables for Folly::follybenchmark and glog::glog - remove explicit linking of std::fs as this is no longer necessary for gcc >= 9 (our minimum is 11) - unify generation and linking of dwrf protobuf files - don't set C++ std for dependencies Pull Request resolved: #11751 Reviewed By: DanielHunte Differential Revision: D66843322 Pulled By: kgpai fbshipit-source-id: a3444a3ff919028523f403e78ade05afb40295c5
assignUser
force-pushed
the
mono-shared
branch
from
c579106 to
9d15e53
Compare
assignUser
force-pushed
the
mono-shared
branch
from
9d15e53 to
5920c93
Compare
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@kgpai, @assignUser Should we wait until #11745 lands? |
Summary: #10732 temporarily changed to a different container. The docker images should be rebuilt with the necessary shared ependencies now so we can witch back. Pull Request resolved: #11796 Reviewed By: Yuhta Differential Revision: D67109007 Pulled By: bikramSingh91 fbshipit-source-id: 174e186a254f6adc866574660a51a7ba1859c372
This introduces the option to build a shared version of the monolithic
libvelox.a, the result is a much smaller build tree, library and executables. They are now so small that moving them is much easier, e.g. for test sharding.I also fixed some miscellaneous things in this PR:
FILESYSTEM, the link is not necessary with GCC >= 9 which is our minimum.FOLLYBENCHMARKwith the proper targetInitially I looked at forcing a bundled build of folly and gflags to make sure they are shared but that introduces weird rpath issues when the dependencies are also on the system (like our CI image), instead I just changed the build script to build folly as shared in the image, we maybe want to make that an option so people can keep folly as static (though even for static build shared folly will likely make the binary size much smaller.). If folly is build from source it will be built static or shared matching the velox build.
I have changed the wrapper function so that only the mono library target is explicitly created as shared, the few other velox utils, test libraries etc. will be build statically to avoid linking and test issues.